feat: custom key comparison / comparator#67
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (37)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduce a pluggable UserComparator and thread a SharedComparator through config, memtables, mergers, iterators, table recovery, tree/version/run logic, compaction, and metadata reads so all internal key comparisons use the configured comparator. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant Tree
participant SuperVersions
participant Memtable
participant Merger
participant Table
participant DataBlock
participant Comparator
Client->>Config: open(config with comparator)
Config->>Tree: create Tree (holds comparator)
Client->>Tree: get(key)
Tree->>SuperVersions: get_internal_entry_from_version(key, seq, comparator)
SuperVersions->>Memtable: point_read(key, seqno, comparator)
Memtable->>Comparator: compare user keys for bounds/ordering
alt Memtable miss
Tree->>Table: point_read/scan(key, comparator)
Table->>DataBlock: iter(comparator)
DataBlock->>Comparator: compare keys during seek/iter
DataBlock-->>Table: InternalValue
end
Table-->>Tree: InternalValue
Tree-->>Client: Value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a pluggable UserComparator so the LSM can order user keys using a caller-defined comparison (instead of hardcoded lexicographic bytes), and threads that comparator through memtables, SST block/index iteration, merge logic, and point-read paths.
Changes:
- Adds the
UserComparatorAPI (withDefaultUserComparator) and aConfig::comparator(...)builder to configure ordering. - Reworks memtable and SST seeking/iteration to use the configured comparator (including block index/data block seek predicates and point reads).
- Adds integration tests for custom comparators (reverse lexicographic + big-endian u64 ordering).
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/range_tombstone_ephemeral.rs | Updates ephemeral memtable construction to pass a comparator. |
| tests/custom_comparator.rs | New integration tests covering custom comparator behavior across reads/iteration and after flush. |
| src/vlog/blob_file/meta.rs | Uses the default comparator for metadata key point-reads after point-read API changes. |
| src/version/super_version.rs | Threads comparator into initial memtable creation in SuperVersions. |
| src/version/run.rs | Adds get_for_key_cmp to select candidate tables using a custom comparator. |
| src/tree/mod.rs | Threads comparator through point-read paths and memtable creation/rotation. |
| src/tree/inner.rs | Initializes SuperVersions with the configured comparator. |
| src/tree/ingest.rs | Passes comparator when opening tables during ingestion. |
| src/table/util.rs | Updates prefixed-slice comparison to delegate to the user comparator (currently via allocation). |
| src/table/tests.rs | Updates table test harness to pass the default comparator to Table open/iter paths. |
| src/table/scanner.rs | Threads comparator into table scanning so block iteration uses the correct ordering. |
| src/table/mod.rs | Stores comparator on Table and uses it for filter/index seeks and data block point reads. |
| src/table/meta.rs | Uses the default comparator for reading metadata keys via point-read. |
| src/table/iter.rs | Threads comparator into table iterators and data-block reader construction. |
| src/table/inner.rs | Adds comparator to the table Inner state. |
| src/table/index_block/mod.rs | Comparator-aware key comparisons for index block parsed items + comparator-carrying iterator. |
| src/table/index_block/iter.rs | Stores comparator in index-block iter and uses it in seek predicates. |
| src/table/data_block/mod.rs | Comparator-aware key comparisons, point reads, and iter construction for data blocks. |
| src/table/data_block/iter_test.rs | Updates data-block iterator tests to pass default comparator. |
| src/table/data_block/iter.rs | Comparator-aware seek predicates and comparisons during data-block iteration. |
| src/table/block_index/volatile.rs | Threads comparator into volatile index iteration/seeking. |
| src/table/block_index/two_level.rs | Threads comparator into two-level index iteration/seeking. |
| src/table/block_index/full.rs | Stores comparator in full block index and uses it when iterating. |
| src/table/block/decoder.rs | Extends ParsedItem::compare_key to accept a comparator. |
| src/range.rs | Switches memtable range iteration to comparator-compatible bounds type (range_internal). |
| src/merge.rs | Comparator-aware heap ordering for merging iterators (adds comparator plumbing). |
| src/memtable/mod.rs | Implements comparator-aware ordering via MemtableKey and adds comparator to memtable state. |
| src/lib.rs | Adds and re-exports the comparator module/types. |
| src/key.rs | Adds InternalKey::compare_with(...) for comparator-based ordering. |
| src/config/mod.rs | Adds comparator field + default + Config::comparator(...) builder. |
| src/comparator.rs | New comparator trait + default implementation + shared comparator type alias. |
| src/compaction/flavour.rs | Passes comparator when opening tables during compaction. |
| src/blob_tree/mod.rs | Threads comparator into point-read path for blob tree index lookups. |
| src/blob_tree/ingest.rs | Passes comparator when opening tables during blob ingestion. |
Comments suppressed due to low confidence (1)
src/range.rs:362
- Tree range iteration merges sources with
Merger::new(iters), which hardcodes the default (lexicographic) comparator. With a customConfig::comparator, this can yield incorrectly ordered merged output and can break range-scan correctness. Thread the tree’s configured comparator intoTreeIterand build the merger viaMerger::with_comparator(iters, comparator.clone())(or equivalent).
let iter = lock.version.active_memtable.range_internal(range.clone());
iters.push(Box::new(
iter.filter(move |item| seqno_filter(item.key.seqno, seqno))
.map(Ok),
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/table/mod.rs (1)
452-462:⚠️ Potential issue | 🔴 CriticalUse the user comparator when ordering recovered range tombstones.
recovernow has the comparator in hand, but the laterrts.sort_unstable_by(|a, b| a.start.cmp(&b.start)...)in this function still hardcodes lexicographic ordering. The point-read/range-tombstone suppression paths rely on that vector being sorted for binary search, so a non-lexicographic comparator can miss a covering tombstone or evaluate them in the wrong order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/table/mod.rs` around lines 452 - 462, The range-tombstone vector is being sorted with lexicographic key ordering even though recover has the user comparator; change the rts.sort_unstable_by call in recover to use the provided comparator (SharedComparator) to compare tombstone start keys instead of a.start.cmp(&b.start) so binary searches and suppression use the user-defined ordering (e.g., replace the closure to call comparator.compare(&a.start, &b.start) and use that Ordering for the sort, adding any necessary tie-breaker using the comparator as well).src/table/data_block/mod.rs (1)
416-460:⚠️ Potential issue | 🟠 MajorHash-based fast paths bypass the comparator's equality relation, risking false negatives with custom comparators.
Table::getusesfilter_block.maybe_contains_hash(key_hash)andDataBlock::point_readuseshash_index_reader.get(needle)— both operating on raw-byte hashes — without checking the comparator type. TheUserComparatortrait does not document thatEqualmust imply bytewise equality; the U64Comparator example shows comparators extracting and comparing u64 values, not comparing bytes directly. If a custom comparator treats byte-different keys as equal, these hash-based shortcuts can produce false negatives: the hash-index lookup returnsMARKER_FREEor the bloom filter rejects the hash before the comparator is ever consulted.Either document that
UserComparator::Equalrequires bytewise equality, or gate the raw-byte hash/bloom shortcuts to the default comparator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/table/data_block/mod.rs` around lines 416 - 460, The hash-based fast-paths must be disabled when a non-default comparator is used to avoid byte-hash vs comparator-equality mismatches: update Table::get (where filter_block.maybe_contains_hash is used) and DataBlock::point_read (where hash_index_reader.get(needle) is used and MARKER_FREE/MARKER_CONFLICT are interpreted) to check the comparator type (e.g., add or call a predicate on the SharedComparator/UserComparator such as is_default() or equivalent) and only use bloom/hash-index shortcuts when the comparator is the default bytewise comparator; otherwise fall back to the existing comparator-aware code paths (seqno-aware binary search / full scan) so the comparator.compare/Equal logic is always consulted for non-default comparators.src/tree/mod.rs (1)
785-823:⚠️ Potential issue | 🔴 CriticalRange tombstone suppression is still byte-ordered.
get_internal_entry_from_versionnow resolves the candidate value withcomparator, but the suppression it calls later in this file still narrows persisted RTs with lexicographic checks (metadata.key_range.contains_key(key)/rt.start.as_ref() <= key). On a non-default comparator tree,remove_rangecan therefore miss keys or suppress unrelated ones on point reads. The RT/key-range checks need the same comparator before this path is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree/mod.rs` around lines 785 - 823, get_internal_entry_from_version uses the user comparator to resolve table candidates but still calls suppression helpers that use byte-ordered checks (e.g., metadata.key_range.contains_key, rt.start.as_ref() <= key), causing incorrect range-tombstone suppression on custom comparators. Fix by making the suppression path comparator-aware: change is_suppressed_by_range_tombstones (and any helpers that check metadata.key_range.contains_key or compare rt.start/rt.end to keys) to accept the same comparator argument and perform comparisons using the comparator's compare/compare_key methods instead of byte-wise ordering, then update calls from get_internal_entry_from_version (and calls from get_internal_entry_from_sealed_memtables/get_internal_entry_from_tables if present) to pass the comparator through. Ensure signatures and call sites are updated consistently.
🧹 Nitpick comments (1)
src/table/util.rs (1)
150-166: Consider avoiding one allocation per prefix-compressed comparison.This helper sits under
DataBlockParsedItem::compare_key, so prefix-compressed seeks/scans now allocate and copy the full user key for every comparison. Reusing a scratch buffer or adding a concat-aware comparator hook would keep the comparator support without turning scans into an allocation-heavy hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/table/util.rs` around lines 150 - 166, The compare_prefixed_slice helper currently allocates a new Vec for every call causing hot-path allocations; modify its API and callers (e.g., DataBlockParsedItem::compare_key) to accept a reusable scratch buffer (e.g., &mut Vec<u8>) or add a new comparator method (e.g., UserComparator::compare_concat(prefix: &[u8], suffix: &[u8], needle: &[u8]) -> Ordering) and call that instead—update compare_prefixed_slice to reuse the provided scratch buffer (or call the new concat-aware comparator) and adjust all call sites to pass the scratch Vec or call the new method so scans no longer allocate per comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/table/data_block/mod.rs`:
- Around line 281-293: compare_key uses three unchecked slice reads
(bytes.get_unchecked(...)) without documented invariants, which is unsound for
malformed on-disk blocks; either validate the recorded ranges when decoding in
parse_full / parse_truncated so compare_key can rely on in-bounds offsets, or
change compare_key to use safe checked slicing (bytes.get(...) or indexing) and
return a sensible error on out-of-bounds; if you retain unsafe gets, add a //
SAFETY: comment directly above each unsafe block explaining the precise
invariant guaranteed by parse_full/parse_truncated (e.g., that
prefix.0..prefix.1 and key.0..key.1 are within bytes) and ensure those
invariants are asserted at decode time.
---
Outside diff comments:
In `@src/table/data_block/mod.rs`:
- Around line 416-460: The hash-based fast-paths must be disabled when a
non-default comparator is used to avoid byte-hash vs comparator-equality
mismatches: update Table::get (where filter_block.maybe_contains_hash is used)
and DataBlock::point_read (where hash_index_reader.get(needle) is used and
MARKER_FREE/MARKER_CONFLICT are interpreted) to check the comparator type (e.g.,
add or call a predicate on the SharedComparator/UserComparator such as
is_default() or equivalent) and only use bloom/hash-index shortcuts when the
comparator is the default bytewise comparator; otherwise fall back to the
existing comparator-aware code paths (seqno-aware binary search / full scan) so
the comparator.compare/Equal logic is always consulted for non-default
comparators.
In `@src/table/mod.rs`:
- Around line 452-462: The range-tombstone vector is being sorted with
lexicographic key ordering even though recover has the user comparator; change
the rts.sort_unstable_by call in recover to use the provided comparator
(SharedComparator) to compare tombstone start keys instead of
a.start.cmp(&b.start) so binary searches and suppression use the user-defined
ordering (e.g., replace the closure to call comparator.compare(&a.start,
&b.start) and use that Ordering for the sort, adding any necessary tie-breaker
using the comparator as well).
In `@src/tree/mod.rs`:
- Around line 785-823: get_internal_entry_from_version uses the user comparator
to resolve table candidates but still calls suppression helpers that use
byte-ordered checks (e.g., metadata.key_range.contains_key, rt.start.as_ref() <=
key), causing incorrect range-tombstone suppression on custom comparators. Fix
by making the suppression path comparator-aware: change
is_suppressed_by_range_tombstones (and any helpers that check
metadata.key_range.contains_key or compare rt.start/rt.end to keys) to accept
the same comparator argument and perform comparisons using the comparator's
compare/compare_key methods instead of byte-wise ordering, then update calls
from get_internal_entry_from_version (and calls from
get_internal_entry_from_sealed_memtables/get_internal_entry_from_tables if
present) to pass the comparator through. Ensure signatures and call sites are
updated consistently.
---
Nitpick comments:
In `@src/table/util.rs`:
- Around line 150-166: The compare_prefixed_slice helper currently allocates a
new Vec for every call causing hot-path allocations; modify its API and callers
(e.g., DataBlockParsedItem::compare_key) to accept a reusable scratch buffer
(e.g., &mut Vec<u8>) or add a new comparator method (e.g.,
UserComparator::compare_concat(prefix: &[u8], suffix: &[u8], needle: &[u8]) ->
Ordering) and call that instead—update compare_prefixed_slice to reuse the
provided scratch buffer (or call the new concat-aware comparator) and adjust all
call sites to pass the scratch Vec or call the new method so scans no longer
allocate per comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d43bb8ca-ad24-4d19-acc0-be8b3cb9331a
📒 Files selected for processing (34)
src/blob_tree/ingest.rssrc/blob_tree/mod.rssrc/compaction/flavour.rssrc/comparator.rssrc/config/mod.rssrc/key.rssrc/lib.rssrc/memtable/mod.rssrc/merge.rssrc/range.rssrc/table/block/decoder.rssrc/table/block_index/full.rssrc/table/block_index/two_level.rssrc/table/block_index/volatile.rssrc/table/data_block/iter.rssrc/table/data_block/iter_test.rssrc/table/data_block/mod.rssrc/table/index_block/iter.rssrc/table/index_block/mod.rssrc/table/inner.rssrc/table/iter.rssrc/table/meta.rssrc/table/mod.rssrc/table/scanner.rssrc/table/tests.rssrc/table/util.rssrc/tree/ingest.rssrc/tree/inner.rssrc/tree/mod.rssrc/version/run.rssrc/version/super_version.rssrc/vlog/blob_file/meta.rstests/custom_comparator.rstests/range_tombstone_ephemeral.rs
polaz
force-pushed
the
feat/#17-feat-custom-key-comparison--comparator
branch
from
48a912c to
3db1e06
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
polaz
force-pushed
the
feat/#17-feat-custom-key-comparison--comparator
branch
2 times, most recently
from
272b524 to
f17a79f
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tree/mod.rs (1)
952-958:⚠️ Potential issue | 🔴 CriticalUse comparator-aware run lookup in merge point reads.
Line 957 still uses
run.get_for_key(key). In this comparator-enabled path, that can miss the correct table under non-lexicographic ordering and drop visible merge/base entries duringget()/multi_get()resolution. Userun.get_for_key_cmp(key, comparator)here.Suggested fix
- if let Some(table) = run.get_for_key(key) { + if let Some(table) = run.get_for_key_cmp(key, comparator) {As per coding guidelines:
**/*.rs: “Flag data corruption bugs: wrong compaction merge logic, incorrect key ordering, dropped or duplicated entries during merge (Tier 1 — MUST flag)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree/mod.rs` around lines 952 - 958, The loop over super_version.version.iter_levels().flat_map(...) is using run.get_for_key(key) which is not comparator-aware and can miss entries; replace that call with run.get_for_key_cmp(key, comparator) in this comparator-enabled path so the run lookup respects non-lexicographic ordering (keep the rest of the logic, including the key_slice range handling, unchanged), ensuring you pass the same comparator in scope and remove the old get_for_key usage.
♻️ Duplicate comments (1)
src/table/data_block/mod.rs (1)
281-295:⚠️ Potential issue | 🔴 CriticalThese on-disk slice offsets are still trusted without validation.
The new comparator-aware path still dereferences
prefix/keywithget_unchecked, butparse_fullandparse_truncatedonly record those ranges—they do not prove that they stay withinbytes. A malformed block can still turn a seek into UB instead of a corruption error.As per coding guidelines, "Flag missing validation: unchecked block offset, unvalidated segment metadata from disk (Tier 1 — MUST flag)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/table/data_block/mod.rs` around lines 281 - 295, compare_key currently uses get_unchecked on offsets recorded by parse_full/parse_truncated (prefix and key), which can lead to UB for malformed on-disk data; change compare_key to validate those ranges before dereferencing (use bytes.get(range) instead of get_unchecked) and propagate a corruption error instead of panicking/UB when a range is out of bounds. Specifically, replace the unsafe slices in compare_key (prefix / self.key usage and calls to compare_prefixed_slice / cmp.compare) with safe lookups, return a Result<Ordering, CorruptionError> (or your crate's existing corruption error type) from compare_key, and update its callers to handle/propagate that error; keep parse_full and parse_truncated as-is (they only record ranges), but rely on the new runtime bounds checks in compare_key to prevent UB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/comparator.rs`:
- Around line 7-33: The Comparator trait lacks a stable persisted identity: add
a stable identifier method (e.g., Comparator::name() or fingerprint()) and
persist that string into the tree metadata when creating a new tree, then update
reopen/recover code paths (config module and Table::recover) to read the stored
comparator identity and compare it to the caller-supplied comparator::name(); if
they differ, return an error and refuse to open/recover the tree. Ensure the
persistence/read happens alongside other tree metadata (creation path in table
creation) and that Table::recover, config loading, and any open/path that
accepts a Comparator check and fail fast on mismatches.
In `@src/memtable/mod.rs`:
- Around line 26-29: The approximate_size calculation still assumes stored
entries are InternalValue-only, but MemtableKey now includes a SharedComparator;
update Memtable::item_size / approximate_size logic to account for the new
MemtableKey layout by adding the size of SharedComparator (or its pointer/arc
overhead) plus MemtableKey::inner to the per-item size calculation; locate
usages around the MemtableKey struct and functions named item_size and
approximate_size and adjust their size math to include comparator overhead so
memtable rotation triggers correctly.
---
Outside diff comments:
In `@src/tree/mod.rs`:
- Around line 952-958: The loop over
super_version.version.iter_levels().flat_map(...) is using run.get_for_key(key)
which is not comparator-aware and can miss entries; replace that call with
run.get_for_key_cmp(key, comparator) in this comparator-enabled path so the run
lookup respects non-lexicographic ordering (keep the rest of the logic,
including the key_slice range handling, unchanged), ensuring you pass the same
comparator in scope and remove the old get_for_key usage.
---
Duplicate comments:
In `@src/table/data_block/mod.rs`:
- Around line 281-295: compare_key currently uses get_unchecked on offsets
recorded by parse_full/parse_truncated (prefix and key), which can lead to UB
for malformed on-disk data; change compare_key to validate those ranges before
dereferencing (use bytes.get(range) instead of get_unchecked) and propagate a
corruption error instead of panicking/UB when a range is out of bounds.
Specifically, replace the unsafe slices in compare_key (prefix / self.key usage
and calls to compare_prefixed_slice / cmp.compare) with safe lookups, return a
Result<Ordering, CorruptionError> (or your crate's existing corruption error
type) from compare_key, and update its callers to handle/propagate that error;
keep parse_full and parse_truncated as-is (they only record ranges), but rely on
the new runtime bounds checks in compare_key to prevent UB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e24eb803-8ec2-497a-be55-f1244c98c10d
📒 Files selected for processing (36)
src/abstract_tree.rssrc/blob_tree/ingest.rssrc/blob_tree/mod.rssrc/compaction/flavour.rssrc/compaction/worker.rssrc/comparator.rssrc/config/mod.rssrc/key.rssrc/lib.rssrc/memtable/mod.rssrc/merge.rssrc/range.rssrc/table/block/decoder.rssrc/table/block_index/full.rssrc/table/block_index/two_level.rssrc/table/block_index/volatile.rssrc/table/data_block/iter.rssrc/table/data_block/iter_test.rssrc/table/data_block/mod.rssrc/table/index_block/iter.rssrc/table/index_block/mod.rssrc/table/inner.rssrc/table/iter.rssrc/table/meta.rssrc/table/mod.rssrc/table/scanner.rssrc/table/tests.rssrc/table/util.rssrc/tree/ingest.rssrc/tree/inner.rssrc/tree/mod.rssrc/version/run.rssrc/version/super_version.rssrc/vlog/blob_file/meta.rstests/custom_comparator.rstests/range_tombstone_ephemeral.rs
✅ Files skipped from review due to trivial changes (3)
- tests/range_tombstone_ephemeral.rs
- src/table/meta.rs
- src/table/index_block/iter.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- src/compaction/flavour.rs
- src/blob_tree/ingest.rs
- src/tree/ingest.rs
- src/tree/inner.rs
- src/blob_tree/mod.rs
- src/range.rs
- src/vlog/blob_file/meta.rs
- src/table/block/decoder.rs
- src/table/scanner.rs
- tests/custom_comparator.rs
- src/table/tests.rs
- src/table/block_index/full.rs
- src/table/data_block/iter.rs
- src/merge.rs
- src/key.rs
- src/config/mod.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/tree/mod.rs:1105
is_suppressed_by_range_tombstonesuses lexicographic ordering for memtable range-tombstone suppression (viaMemtable::is_key_suppressed_by_range_tombstone/ interval_tree), but uses the configuredUserComparatorfor SST range-tombstone suppression below. With a non-lexicographic comparator, this makes suppression semantics differ before vs after flush/compaction, which can lead to inconsistent point-read results. Consider threading the comparator through memtable range-tombstone queries (and using it for interval-tree ordering/containment), or keep the SST suppression path lexicographic until the whole range-tombstone stack is comparator-aware.
polaz
force-pushed
the
feat/#17-feat-custom-key-comparison--comparator
branch
from
4c7c8bd to
daad3e0
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
…FETY docs - Restore key_range filter in is_suppressed_by_range_tombstones using comparator-aware comparison instead of lexicographic KeyRange::contains_key - Add SAFETY comments on all unsafe get_unchecked calls in compare_prefixed_slice_lexicographic explaining the min() bound - Document slow-path allocation rationale for custom comparators
- Config::comparator: pub -> pub(crate), builder method is the public API - Document get_for_key_cmp: explains why comparator-based binary search is correct for runs sorted in comparator order
- decode_range_tombstones: validate start < end using comparator instead of lexicographic Ord - Add SAFETY comment on final get_unchecked in compare_prefixed_slice_lexicographic explaining rest_len guard - Add unit test for custom-comparator slow path (ReverseComparator)
polaz
force-pushed
the
feat/#17-feat-custom-key-comparison--comparator
branch
from
fd4cd54 to
0efc392
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Closed
5 tasks
6 tasks
polaz
added a commit
that referenced
this pull request
Mar 23, 2026
## Summary - Add `name() -> &'static str` method to `UserComparator` trait for stable comparator identity - Persist comparator name in tree manifest; check on reopen — mismatch returns `Error::ComparatorMismatch` - Backward compatible: trees created before this change default to `"default"` (matching `DefaultUserComparator`) ## Technical Details - Comparator name written as `comparator_name` section in sfa archive during `persist_version` - `SuperVersions` stores `comparator_name: Arc<str>` so flush/compaction version upgrades include it without extra plumbing - Check runs in `Tree::recover` after manifest decode, before any data access - Follows RocksDB `Comparator::Name()` pattern (requested in #67 review) ## Test Plan - [x] Reopen with same comparator succeeds - [x] Reopen with different custom comparator fails with `ComparatorMismatch` - [x] Reopen custom-comparator tree with default comparator fails - [x] Reopen default-comparator tree with default comparator succeeds - [x] All existing tests pass (429 unit + integration) Closes #74 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** - Tree comparators are now persisted and automatically validated when reopening a tree. * **Bug Fixes** - Attempting to reopen a tree with an incompatible comparator now fails with a clear error message. * **Tests** - Added comprehensive tests for comparator persistence and validation behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UserComparatortrait for custom key ordering instead of hardcoded lexicographic byte comparisonTechnical Details
New public API:
UserComparatortrait —compare(&self, a: &[u8], b: &[u8]) -> Ordering+is_lexicographic()for fast-path detectionDefaultUserComparator— lexicographic bytes (backward compatible default)Config::comparator(Arc<dyn UserComparator>)— builder method (field ispub(crate))compare(a, b) == Equalmust implya == b(bloom/hash rely on this)Threading strategy:
MemtableKeywrapper carriesSharedComparatorforSkipMaporderingParsedItem::compare_keyaccepts&dyn UserComparator;compare_prefixed_slicehas zero-alloc fast path for lexicographic comparatorsHeapItemusesInternalKey::compare_with;Merger::newrequires explicit comparatorRun::get_for_key_cmpfor correct table selectionis_suppressed_by_range_tombstonesuses comparator for key-range filter and containmentSharedComparator, use in seek predicatesdefault_comparator()viaLazyLockavoids repeated Arc allocationsKnown limitations:
Ord— RT suppression in memtable may be incorrect with non-lexicographic comparators (tracked as follow-up issue)KeyRangecomparisons in some compaction paths still use lexicographic orderingTest Plan
cargo clippycleanCloses #17
Summary by CodeRabbit
New Features
API Changes
Tests